Size Change: +75 B (0%)

Total Size: 1.44 MB

_{compressed-size-action}

Flaky tests detected in d9d4f23.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5306943596
📝 Reported issues:

• [Flaky Test] Pick the additional variation in the inserted Columns block #47057 in specs/editor/plugins/block-variations.test.js
• [Flaky Test] page-list submenu user interactions #51348 in /test/e2e/specs/editor/blocks/navigation.spec.js

Thanks for the review, @aaronrobertshaw!

I added the supports property to deprecated and also added a new fixture. Both the fixture I added (core__details__deprecated-v0.html) and the fixture that originally existed (core__details.html) have exactly the same result, but this is correct, right?

Thanks for the review, @aaronrobertshaw!

I found two things to worry about after merging the latest trunk.

• By Fix default block dimensions visibility #52256, __experimentalDefaultControls was added to block.json. it seems that the migration will run even if the deprecated supports are not updated, but Should __experimentalDefaultControls be added to support?
• Create a Details block in trunk and switch to this PR. I noticed that when I reload the editor, no logs appear in the browser console and no migration takes place. However, when I update the content, the migration takes place. Is this related to the fact that I did not update save.js, only block.json?

By #52256, __experimentalDefaultControls was added to block.json. it seems that the migration will run even if the deprecated supports are not updated, but Should __experimentalDefaultControls be added to support?

Changes to block supports would only need to trigger a migration if they impact the saved markup for the block. In this case, the support changes are only to control what UI controls are displayed by default. Essentially, __experimentalDefaultControls doesn't impact the markup at all and as such doesn't need a deprecation.

This PR is introducing the first deprecation for the Details block. It should reflect the current state of the attributes and supports at the time it lands. This would include the __experimentalDefaultControls change made recently. Even though it doesn't impact the markup to omit it, it will reduce confusion when someone comes to add further deprecations in the future. Deprecations are a bit of a nightmare at times as they are without adding to it 😅

I noticed that when I reload the editor, no logs appear in the browser console and no migration takes place. However, when I update the content, the migration takes place. Is this related to the fact that I did not update save.js, only block.json?

If I'm following correctly, the question is why doesn't the block migrate without editing it?

My guess would be that the block markup is still valid as it is only the comment delimiter that is changing. This means the block isn't invalidated.

Otherwise, valid blocks can still trigger a deprecation to run if their isEliigble function returns true. You might then be able to use the deprecation's migrate function to clear the summary attribute.

I quickly hacked up something locally but ran out of time to get it working properly. The snippet below does not appear to be updating the block delimiter despite clearing the summary attribute. It also then incorrectly shows the "Write summary" placeholder.

Unfortunately, I'm out of time this week so might have to leave this one with you.

diff --git a/packages/block-library/src/details/deprecated.js b/packages/block-library/src/details/deprecated.js
index e64bf11ce16..35181fa1588 100644
--- a/packages/block-library/src/details/deprecated.js
+++ b/packages/block-library/src/details/deprecated.js
@@ -48,6 +48,12 @@ const v0 = {
 			},
 		},
 	},
+	migrate: ( attributes, innerBlocks ) => [
+		{ ...attributes, summary: undefined },
+		innerBlocks,
+	],
+	isEligible: ( attributes, innerBlocks, { blockNode } ) =>
+		!! blockNode.attrs?.summary,
 	save( { attributes } ) {
 		const { showContent } = attributes;
 		const summary = attributes.summary ? attributes.summary : 'Details';

Having the summary attribute within the block delimiter doesn't break things. Any new details block, or any existing one that gets updated, will have the attribute cleared from the block delimiter. Perhaps, we should avoid the deprecation altogether?

Some fresh eyes and opinions on this would be welcome cc/ @andrewserong

Thanks for the ping!

Having the summary attribute within the block delimiter doesn't break things. Any new details block, or any existing one that gets updated, will have the attribute cleared from the block delimiter. Perhaps, we should avoid the deprecation altogether?

Yes, in this case because the block markup hasn't changed, my understanding is that we wouldn't need a deprecation here. I don't think there's a need for us to migrate old comment markup to the new format — as you mention, we can happily change what gets serialised when the content of the block is manually updated by the user, rather than forcing it via a deprecation.

I like the idea of this PR, though, thanks for working on it @t-hamano! It seems better overall to me to not store a duplicate of the data in the comment delimiter 👍

Thank you for the detailed explanation, @aaronrobertshaw, @andrewserong!

If I understand correctly, this PR only requires the addition of the attributes selector and source in block.json, correct? I have updated the PR title, description, and code, but if I have misunderstood anything please let me know 🙇

Thanks for the review, @aaronrobertshaw, @andrewserong!